fix: propagate fetch reasons for interface-related fields by ysmolski · Pull Request #1312 · wundergraph/graphql-go-tools

ysmolski · 2025-10-07T13:30:56Z

Interfaces were not handled before at all.
Now, if interface field is marked with special directive,
then fields on implementing type are considered as marked too.

You can find more details in the comments.

Summary by CodeRabbit

Bug Fixes
- Improved propagation of fetch reasons across interfaces, objects and fragments so subrequest payloads reliably include all relevant implementing types and avoid duplicates.
Tests
- Expanded coverage for interface/object/fragment combinations, multiple implementers, single-implementer cases, and duplicate-avoidance scenarios.
Refactor
- Internal fetch-reason propagation logic reworked for clearer, deduplicated results and consistent sorting.
Chores (breaking)
- Simplified public metadata shape by removing a previously exported fetch-reason field from root-node metadata (may require client updates).

coderabbitai · 2025-10-07T13:31:21Z

Walkthrough

Tests expanded to validate fetch-reason propagation across interfaces and implementing types; RootNodes metadata no longer includes FetchReasonFields. Visitor replaced inline propagation with a new helper that deduplicates, merges, and extends fetch reasons to object/interface fields. A doc comment in resolve/fetch.go was adjusted.

Changes

Cohort / File(s)	Summary
Execution engine tests `execution/engine/execution_engine_test.go`	Expanded and relabeled tests to cover interface-based fetch reasons across multiple subgraphs; updated expected HTTP subrequest bodies and data plan metadata (RootNodes entries omit `FetchReasonFields`; ChildNodes retain interface-related entries); added scenarios for various interface/object/fragment combinations and duplicate-avoidance.
Planner visitor: fetch-reason propagation `v2/pkg/engine/plan/visitor.go`	Replaced inline propagation logic with `getPropagatedReasons(fetchID, fetchReasons)` on `Visitor`; added dedup/merge of `ByUser`/`BySubgraphs`, preserved `IsKey`/`IsRequires`, propagated interface fields to implementing object fields (and vice versa where applicable), introduced `cmpFetchReasons` comparator, and used it for sorting in `buildFetchReasons`.
Resolve package docs `v2/pkg/engine/resolve/fetch.go`	Minor documentation wording change on `FetchReason` coordinates (`(TypeName, FieldName)`); no behavioral changes.

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~60 minutes

Possibly related PRs

fix: refactor CoordinateDependencies, FetchReasons #1293 — Modifies fetch-reason construction and propagation logic; overlaps with visitor propagation changes in this PR.
feat: validate presence of optional @requires dependencies #1297 — Changes build/propagation of fetch reasons and FetchReason shape; touches same code paths in Visitor and fetch-reason handling.
feat: add support of field selection reasons extensions #1282 — Alters per-field fetch-reason propagation for interfaces/concrete types; directly related to the new propagation helper and metadata shape changes.

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%.	You can run `@coderabbitai generate docstrings` to improve docstring coverage.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title Check	✅ Passed	The title succinctly captures the core change by indicating that fetch reasons are now propagated for interface‐related fields, which aligns directly with the modifications in fetch reason propagation logic and tests for interfaces.

✨ Finishing touches

📝 Generate docstrings

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between b605de8 and 7e9d0f2.

📒 Files selected for processing (1)

v2/pkg/engine/plan/visitor.go (4 hunks)

🧰 Additional context used

🧬 Code graph analysis (1)

v2/pkg/engine/plan/visitor.go (3)

v2/pkg/engine/resolve/fetch.go (1)

FetchReason (364-372)

v2/pkg/engine/plan/federation_metadata.go (1)

FieldCoordinate (89-92)

pkg/ast/ast_node_kind.go (2)

NodeKindObjectTypeDefinition (14-14)

NodeKindInterfaceTypeDefinition (16-16)

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)

GitHub Check: Build and test (go 1.25 / windows-latest)
GitHub Check: Build and test (go 1.25 / ubuntu-latest)
GitHub Check: Build and test (go 1.25 / ubuntu-latest)
GitHub Check: Build and test (go 1.25 / windows-latest)

🔇 Additional comments (2)

v2/pkg/engine/plan/visitor.go (2)

1509-1514: LGTM! Well-structured comparison helper.

The new cmpFetchReasons function provides a clean, reusable comparator for sorting fetch reasons by type and field name. Using cmp.Or and cmp.Compare is idiomatic Go 1.21+ style. This helper is correctly utilized at line 1505 (in buildFetchReasons) and line 1624 (in getPropagatedReasons), eliminating duplication.

1528-1626: Excellent interface propagation implementation—past issues resolved.

The getPropagatedReasons method correctly implements fetch reason propagation with proper interface handling:

Deduplication & merging: The appendOrMerge helper correctly deduplicates entries by field coordinate and merges ByUser, BySubgraphs, IsKey, and IsRequires (lines 1553-1567).

Object→Interface propagation: Lines 1584-1595 correctly propagate reasons when an object field isn't in the lookup but its interface field is.

Interface→Object propagation: Lines 1597-1622 correctly extend interface field reasons to all implementing type fields, handling both cases where the interface field or implementing field is in the lookup.

Past issues addressed:

✅ Lines 1558-1559 now sort and compact BySubgraphs after merging (previously flagged in past review)

✅ Line 1617 clones BySubgraphs to prevent shared slice corruption (previously flagged in past review)

The method is properly invoked at line 1351 to populate PropagatedFetchReasons in the fetch configuration.

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

coderabbitai

Actionable comments posted: 0

🧹 Nitpick comments (2)

v2/pkg/engine/plan/visitor.go (1)
1529-1621: Propagates interface/object reasons correctly; add BySubgraphs dedup/sort

Logic covers:

Direct lookup match,

Object field via interface-marked fallback,

Interface → all implementing types (or only those in lookup).
Dedup via FieldCoordinate index is solid.

Missing: after merges, BySubgraphs may contain duplicates and non-deterministic order. Sort+compact for each reason (as in buildFetchReasons) to keep payload stable.

Apply this at the end of the function:
 func (v *Visitor) getPropagatedReasons(fetchID int, fetchReasons []resolve.FetchReason) []resolve.FetchReason {
@@
-    slices.SortFunc(propagated, cmpFetchReasons)
+    // Normalize merged subgraph lists for determinism
+    for i := range propagated {
+        if len(propagated[i].BySubgraphs) > 0 {
+            slices.Sort(propagated[i].BySubgraphs)
+            propagated[i].BySubgraphs = slices.Compact(propagated[i].BySubgraphs)
+        }
+    }
+    slices.SortFunc(propagated, cmpFetchReasons)
     return propagated
 }
execution/engine/execution_engine_test.go (1)

1096-1166: Consider adding a BySubgraphs merge test

You test dedupe across interface/implementing reasons. Add one case where a coordinate is requested by multiple subgraphs to assert BySubgraphs is deduped and sorted (e.g., ["a","b","b"] → ["a","b"]). This hardens determinism of extensions.fetch_reasons.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 0d60605 and 4689e83.

📒 Files selected for processing (3)

execution/engine/execution_engine_test.go (3 hunks)
v2/pkg/engine/plan/visitor.go (4 hunks)
v2/pkg/engine/resolve/fetch.go (1 hunks)

🧰 Additional context used

🧬 Code graph analysis (2)

v2/pkg/engine/plan/visitor.go (3)

v2/pkg/engine/resolve/fetch.go (1)

FetchReason (364-372)

v2/pkg/engine/plan/federation_metadata.go (1)

FieldCoordinate (89-92)

pkg/ast/ast_node_kind.go (2)

NodeKindObjectTypeDefinition (14-14)

NodeKindInterfaceTypeDefinition (16-16)

execution/engine/execution_engine_test.go (3)

v2/pkg/engine/plan/datasource_configuration.go (2)

DataSourceMetadata (34-59)

DataSource (274-287)

v2/pkg/engine/plan/type_field.go (1)

TypeField (3-8)

v2/pkg/engine/datasource/graphql_datasource/configuration.go (3)

ConfigurationInput (16-23)

FetchConfiguration (124-128)

SchemaConfiguration (135-139)

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)

GitHub Check: Build and test (go 1.25 / windows-latest)
GitHub Check: Build and test (go 1.25 / ubuntu-latest)
GitHub Check: Build and test (go 1.25 / windows-latest)
GitHub Check: Build and test (go 1.25 / ubuntu-latest)

🔇 Additional comments (6)

v2/pkg/engine/resolve/fetch.go (1)

362-372: Docstring alignment looks good

Comment reflects (TypeName, FieldName) coordinates and matches struct fields and JSON keys.

v2/pkg/engine/plan/visitor.go (4)

1351-1352: Correct placement of propagated reasons assignment

Setting PropagatedFetchReasons only when FetchReasons exist is safe and avoids nils.

1421-1423: Index by FieldCoordinate is appropriate

Using a typed key improves safety and avoids stringly-typed map keys. Good.

1505-1506: Deterministic ordering via cmpFetchReasons

Switch to a reusable comparator improves consistency.

1509-1515: cmpFetchReasons is correct and minimal

Lexicographic TypeName, then FieldName comparison is sufficient for stable ordering.

execution/engine/execution_engine_test.go (1)

846-904: Great coverage for interface propagation to implementing types

Validates Character.name marking → Character, Droid, Human reasons and POST body composition. LGTM.

… yury/eng-8266-support-requirefetchreasons-on-interface-fields

coderabbitai

Actionable comments posted: 2

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 4689e83 and b605de8.

📒 Files selected for processing (1)

v2/pkg/engine/plan/visitor.go (4 hunks)

🧰 Additional context used

🧬 Code graph analysis (1)

v2/pkg/engine/plan/visitor.go (3)

v2/pkg/engine/resolve/fetch.go (1)

FetchReason (364-372)

v2/pkg/engine/plan/federation_metadata.go (1)

FieldCoordinate (89-92)

pkg/ast/ast_node_kind.go (2)

NodeKindObjectTypeDefinition (14-14)

NodeKindInterfaceTypeDefinition (16-16)

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)

GitHub Check: Build and test (go 1.25 / windows-latest)
GitHub Check: Build and test (go 1.25 / ubuntu-latest)
GitHub Check: Build and test (go 1.25 / ubuntu-latest)
GitHub Check: Build and test (go 1.25 / windows-latest)

🔇 Additional comments (4)

v2/pkg/engine/plan/visitor.go (4)

1351-1351: LGTM!

The integration of the new getPropagatedReasons helper is clean and appropriate.

1421-1422: LGTM!

The comment clearly explains the purpose of the index map.

1505-1505: LGTM!

Good refactor to use the shared cmpFetchReasons helper for consistency.

1509-1514: LGTM!

The comparison logic is correct and idiomatic. It properly compares by TypeName first, then FieldName for consistent sorting.

🤖 I have created a release *beep* *boop* --- ## [2.0.0-rc.230](v2.0.0-rc.229...v2.0.0-rc.230) (2025-10-09) ### Bug Fixes * avoid duplicate joins on errors ([#1314](#1314)) ([a1f1f8c](a1f1f8c)) * propagate fetch reasons for interface-related fields ([#1312](#1312)) ([5ee3014](5ee3014)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).  ## Summary by CodeRabbit * **Bug Fixes** * Prevent duplicate joins when errors occur, improving stability. * Propagate fetch reasons for interface-related fields for more accurate behavior and diagnostics. * **Documentation** * Added changelog entry for version 2.0.0-rc.230. * **Chores** * Bumped version to 2.0.0-rc.230.

@requires

🤖 I have created a release *beep* *boop* --- ## [1.6.0](execution/v1.5.0...execution/v1.6.0) (2025-10-21) ### Features * support the oneOf directive ([#1308](#1308)) ([251cb02](251cb02)) * validate presence of optional [@requires](https://github.com/requires) dependencies ([#1297](#1297)) ([ba75e25](ba75e25)) ### Bug Fixes * bump engine to v2.0.0-rc.231 for execution ([#1329](#1329)) ([ebddb25](ebddb25)) * propagate fetch reasons for interface-related fields ([#1312](#1312)) ([5ee3014](5ee3014)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).  ## Summary by CodeRabbit * **New Features** * Added oneOf directive support * Added validation of optional @requires dependencies * **Bug Fixes** * Fixed fetch reason propagation for interface-related fields

ysmolski added 3 commits October 3, 2025 16:17

draft the implementation for interfaces

5a7ad58

make first working version

5889624

deduplicate and merge propagated fetch reasons

4689e83

ysmolski requested review from a team, Noroth, StarpTech, devsergiy and jensneuse as code owners October 7, 2025 13:30

coderabbitai Bot reviewed Oct 7, 2025

View reviewed changes

ysmolski changed the title ~~fix: support requireFetchReasons on interface fields~~ fix: propagate fetch reasons for interface-related fields Oct 7, 2025

devsergiy approved these changes Oct 9, 2025

View reviewed changes

Comment thread v2/pkg/engine/plan/visitor.go Outdated

ysmolski added 2 commits October 9, 2025 15:06

rename a var

9667f9d

Merge branch 'master' of github.com:wundergraph/graphql-go-tools into…

b605de8

… yury/eng-8266-support-requirefetchreasons-on-interface-fields

coderabbitai Bot reviewed Oct 9, 2025

View reviewed changes

Comment thread v2/pkg/engine/plan/visitor.go

Comment thread v2/pkg/engine/plan/visitor.go

sort and dedupe the BySubgraphs slice

7e9d0f2

ysmolski merged commit 5ee3014 into master Oct 9, 2025
11 checks passed

ysmolski deleted the yury/eng-8266-support-requirefetchreasons-on-interface-fields branch October 9, 2025 13:42

This was referenced Oct 9, 2025

chore(master): release execution 1.6.0 #1303

Merged

chore(master): release 2.0.0-rc.230 #1317

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: propagate fetch reasons for interface-related fields#1312

fix: propagate fetch reasons for interface-related fields#1312
ysmolski merged 6 commits intomasterfrom
yury/eng-8266-support-requirefetchreasons-on-interface-fields

ysmolski commented Oct 7, 2025 •

edited by coderabbitai Bot

Loading

Uh oh!

coderabbitai Bot commented Oct 7, 2025 •

edited

Loading

Uh oh!

coderabbitai Bot left a comment

Uh oh!

Uh oh!

coderabbitai Bot left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

ysmolski commented Oct 7, 2025 • edited by coderabbitai Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary by CodeRabbit

Uh oh!

coderabbitai Bot commented Oct 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Changes

Estimated code review effort

Possibly related PRs

Pre-merge checks and finishing touches

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

ysmolski commented Oct 7, 2025 •

edited by coderabbitai Bot

Loading

coderabbitai Bot commented Oct 7, 2025 •

edited

Loading